Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed create push notification with external_id #74

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fixed create push notification with external_id #74

wants to merge 1 commit into from

Conversation

GizemSever
Copy link

@GizemSever GizemSever commented Jan 3, 2024 •
edited
Loading

Description

  • Fixed create push notification with external_id, onesignal_id

Details

  • createNotification function is not send include_aliases object with extearnal_id values

Motivation

  • Fixes a specific bug,

Testing

Manual testing

  • Creating push notifications using external_id value was tested.

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

@GizemSever GizemSever mentioned this pull request Jan 3, 2024
Closed
1 task
@ayokay123
Copy link

Any updates?

@ansh
Copy link

ansh commented Jan 31, 2024

What is the update on this? This literally does not work. What a joke.

@nan-li
Copy link
Contributor

nan-li commented Jan 31, 2024 •
edited
Loading

Hi @GizemSever, thank you for submitting this PR and include_aliases is indeed not working as expected.

We are working on overhauling the APIs and interfaces to be updated and correct, and the fix for include_aliases will be included.

In the meantime, it is still safe to use include_external_user_ids to send notifications to target by External ID, and include_player_ids to target by Subscription ID, instead of using include_aliases.

This library is actually generated from a primary spec via the OpenAPI generator so no code in this library is updated manually. In addition, include_aliases needs to be more generic since developers can have any string as an alias label, not just "onesignal_id" or "external_id".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PauwelsPieter PauwelsPieter PauwelsPieter approved these changes

@Jackmekiss Jackmekiss Jackmekiss approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@GizemSever @ayokay123 @ansh @nan-li @Jackmekiss @PauwelsPieter